-
Notifications
You must be signed in to change notification settings - Fork 1k
Fixing merge #300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing merge #300
Conversation
- Removed DisallowsSameSiteNone from WebAppServiceCollectionExtensions because it got moved to another class - Added edit profile button on login bar - Fixed merge on AuthorizeForScopesAttribute - Added options.TokenValidationParameters.NameClaimType = "name" on web api Startup to fix todo list
@jmprieur @jennyf19 I have a few observations that I would like to discuss: 1- We have some sort of method duplication in AuthorityHelpers.cs 2- Could you confirm if the following behavior is expected?
3- For B2C, the remove account might have a bug. Since one user can have many IAccounts (because of the policies), we need to find a solution that gets all of the accounts of that user (regardless the policy) and remove from the cache. The current implementation is not doing it. 4- The current port used in the client app is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @TiagoBrenck
@TiagoBrenck : we'll answer your questions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks @TiagoBrenck
We also need to update the Web APIs so that they check the scopes (they don't at the moment)
Are you talking about this check? options.Events.OnTokenValidated = async context =>
{
// This check is required to ensure that the Web API only accepts tokens from tenants where it has been consented and provisioned.
if (!context.Principal.Claims.Any(x => x.Type == ClaimConstants.Scope)
&& !context.Principal.Claims.Any(y => y.Type == ClaimConstants.Scp)
&& !context.Principal.Claims.Any(y => y.Type == ClaimConstants.Roles)
&& !context.Principal.Claims.Any(y => y.Type == ClaimConstants.Role))
{
throw new UnauthorizedAccessException("Neither scope or roles claim was found in the bearer token.");
}
await tokenValidatedHandler(context).ConfigureAwait(false);
}; |
No, this verifies that there are either roles or scopes, but we also need to have the controller actions verify the scopes. |
@TiagoBrenck. here are the answers to your questions
I've extracted EnsureAuthorityIsV2_0 from the AddXX method so that we can unit test it. I think it should stay there. It's unit tested. it's also more robust as it process correctly authorities that end in / or not. We cannot, tough use
I think it's fine (it works functionally?). We might want to raise an issue in the sample to fix it later.
Good catch. I've raised an issue for this AzureAD/microsoft-identity-web#20
Let's update the documentation (we have the app registration right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions;
/// <summary> | ||
/// AuthorizationHandler that will check if the scope claim has the requirement value | ||
/// </summary> | ||
public class OperationScopeHandler : AuthorizationHandler<OperationAuthorizationRequirement> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we be able to specialize OperationAuthorizationRequirement
to ScopeRequirement
and have a property named RequiredScope
?
How do we allow several scopes (for instance a given method can be called if the scope is user.read or user.write?
{ | ||
// Create policy to check for the scope 'read' | ||
options.AddPolicy("ReadScope", | ||
policy => policy.Requirements.Add(new OperationAuthorizationRequirement { Name = "read" })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to be able to write new OperationScopeRequirement {RequiredScope = "user.read"}
4-WebApp-your-API/4-2-B2C/TodoListService/AuthorizationPolicies/ScopesRequirement.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
left a few suggestions
Thanks @TiagoBrenck |
Purpose
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?